Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: interception id not found error in route.continue #29180

Merged
merged 2 commits into from
Jan 26, 2024

Conversation

yury-s
Copy link
Member

@yury-s yury-s commented Jan 25, 2024

We stopped catching all exceptions in #28539 in hope that we'll get loadingFailed even before Fetch.continue/fulfill command's error. Turns out this is racy and may fail if the test cancels the request while we are continuing it. The following test could in theory reproduce it if stars align and the timing is good:

it('page.continue on canceled request', async ({ page }) => {
  let resolveRoute;
  const routePromise = new Promise<Route>(f => resolveRoute = f);
  await page.route('http://test.com/x', resolveRoute);

  const evalPromise = page.evaluate(async () => {
    const abortController = new AbortController();
    (window as any).abortController = abortController;
    return fetch('http://test.com/x', { signal: abortController.signal }).catch(e => 'cancelled');
  });
  const route = await routePromise;
  void page.evaluate(() => (window as any).abortController.abort());
  await new Promise(f => setTimeout(f, 10));
  await route.continue();
  const req = await evalPromise;
  expect(req).toBe('cancelled');
});

Fixes #29123

This comment has been minimized.

Copy link
Contributor

Test results for "tests 1"

8 flaky ⚠️ [chromium] › library/tracing.spec.ts:243:5 › should not include trace resources from the previous chunks
⚠️ [chromium] › library/tracing.spec.ts:243:5 › should not include trace resources from the previous chunks
⚠️ [chromium] › library/tracing.spec.ts:243:5 › should not include trace resources from the previous chunks
⚠️ [firefox] › library/tracing.spec.ts:243:5 › should not include trace resources from the previous chunks
⚠️ [firefox] › page/page-goto.spec.ts:431:3 › js redirect overrides url bar navigation
⚠️ [chromium] › library/tracing.spec.ts:243:5 › should not include trace resources from the previous chunks
⚠️ [playwright-test] › ui-mode-test-screencast.spec.ts:21:5 › should show screenshots
⚠️ [webkit] › library/tracing.spec.ts:243:5 › should not include trace resources from the previous chunks

21359 passed, 581 skipped
✔️✔️✔️

Merge workflow run.

@yury-s yury-s merged commit 82981a8 into microsoft:main Jan 26, 2024
28 of 30 checks passed
@yury-s yury-s deleted the route-continue-ex branch January 26, 2024 00:54
@purepear
Copy link

Hi guys, thanks for taking care of this issue! 🙇

Do you know if this will be released as a patch 1.41.2 or minor 1.42.0 release? (we started noticing some flakiness and we not sure if we should wait for 1.41.2 or downgrade until 1.42 is out)

yury-s added a commit to yury-s/playwright that referenced this pull request Jan 29, 2024
… route.continue

We stopped catching all exceptions in
microsoft#28539 in hope that we'll
get loadingFailed even before Fetch.continue/fulfill command's error.
Turns out this is racy and may fail if the test cancels the request
while we are continuing it. The following test could in theory reproduce
it if stars align and the timing is good:

```js
it('page.continue on canceled request', async ({ page }) => {
  let resolveRoute;
  const routePromise = new Promise<Route>(f => resolveRoute = f);
  await page.route('http://test.com/x', resolveRoute);

  const evalPromise = page.evaluate(async () => {
    const abortController = new AbortController();
    (window as any).abortController = abortController;
    return fetch('http://test.com/x', { signal: abortController.signal }).catch(e => 'cancelled');
  });
  const route = await routePromise;
  void page.evaluate(() => (window as any).abortController.abort());
  await new Promise(f => setTimeout(f, 10));
  await route.continue();
  const req = await evalPromise;
  expect(req).toBe('cancelled');
});
```

Fixes microsoft#29123
@yury-s
Copy link
Member Author

yury-s commented Jan 29, 2024

Hi guys, thanks for taking care of this issue! 🙇

Do you know if this will be released as a patch 1.41.2 or minor 1.42.0 release? (we started noticing some flakiness and we not sure if we should wait for 1.41.2 or downgrade until 1.42 is out)

We'll cherry-pick the fix and push it in 1.41.2.

yury-s added a commit that referenced this pull request Jan 29, 2024
#29222)

…ntinue

We stopped catching all exceptions in
#28539 in hope that we'll
get loadingFailed even before Fetch.continue/fulfill command's error.
Turns out this is racy and may fail if the test cancels the request
while we are continuing it. The following test could in theory reproduce
it if stars align and the timing is good:

```js
it('page.continue on canceled request', async ({ page }) => {
  let resolveRoute;
  const routePromise = new Promise<Route>(f => resolveRoute = f);
  await page.route('http://test.com/x', resolveRoute);

  const evalPromise = page.evaluate(async () => {
    const abortController = new AbortController();
    (window as any).abortController = abortController;
    return fetch('http://test.com/x', { signal: abortController.signal }).catch(e => 'cancelled');
  });
  const route = await routePromise;
  void page.evaluate(() => (window as any).abortController.abort());
  await new Promise(f => setTimeout(f, 10));
  await route.continue();
  const req = await evalPromise;
  expect(req).toBe('cancelled');
});
```

Fixes #29123
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] route.continue: Protocol error (Fetch.continueRequest): Invalid InterceptionId.
3 participants